Open
Conversation
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
|
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
📝 WalkthroughWalkthroughThis PR refactors network modification handling by introducing a dedicated enum type for composite modification actions, removing the Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.1)src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/NetworkModificationTest.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
Contributor
Author
They follow exactly the same path so I don't think a specific test would bring anything. Actually |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java (1)
260-277:⚠️ Potential issue | 🔴 CriticalFail fast when the remote mutation response is empty.
These methods all return
exchange(...).getBody()directly, but the new callers immediately consume duplicated UUIDs and impact results from that body. A successfulPUTwith no payload will now surface later as a local NPE/IOOBE after the remote write already happened.Suggested guard at the client boundary
- return restTemplate.exchange( + NetworkModificationsResult result = restTemplate.exchange( getNetworkModificationServerURI(false) + path.buildAndExpand(targetGroupUuid).toUriString(), HttpMethod.PUT, httpEntity, - NetworkModificationsResult.class).getBody(); + NetworkModificationsResult.class).getBody(); + return Objects.requireNonNull(result, "Expected NetworkModificationsResult body");Apply the same pattern to the other changed methods that currently return
getBody()directly.Also applies to: 280-300, 303-321, 344-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java` around lines 260 - 277, The REST client call in moveModifications currently returns restTemplate.exchange(...).getBody() directly which can be null and cause NPE/IOOBE downstream; update moveModifications (and the other similar methods that call restTemplate.exchange(...).getBody()) to capture the ResponseEntity<NetworkModificationsResult>, check responseEntity.getBody() for null, and if null throw a clear, fast-fail exception (e.g., IllegalStateException or a custom exception) with a descriptive message indicating the remote PUT returned an empty body for the MOVE action and include context (originGroupUuid, targetGroupUuid); ensure you reference the restTemplate.exchange call and NetworkModificationsResult so the guard is added at the client boundary for all analogous methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/gridsuite/study/server/controller/StudyController.java`:
- Around line 689-692: The endpoint StudyController.insertCompositeModifications
currently accepts a RequestBody List of org.springframework.data.util.Pair which
Jackson cannot deserialize; create a dedicated DTO (e.g.,
CompositeModificationInfo with fields uuid and name) and replace the parameter
type modificationsToInsert from List<Pair<UUID,String>> to
List<CompositeModificationInfo>, update any imports/usages and OpenAPI
annotations accordingly, and ensure the new DTO is a public record or class with
JVM-accessible properties so Jackson can serialize/deserialize it without custom
deserializers.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2424-2426: The call to
networkModificationService.duplicateModifications(...) assumes the response has
one duplicated UUID per requested source and then blindly calls
copyModificationsToExclude; update the code around duplicateModifications and
copyModificationsToExclude to first validate that NetworkModificationsResult (or
its duplicates list/map) contains exactly the same number of entries as
modificationsUuids (and that each source UUID maps to a duplicate UUID), and if
the sizes/mapping do not match, fail fast (throw an exception or return an
error) or log and skip to avoid misapplying exclusion tags; apply the same
validation to the other block referenced (around the 2476-2485 area) so you
always map source->duplicate deterministically rather than assuming order.
---
Outside diff comments:
In
`@src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java`:
- Around line 260-277: The REST client call in moveModifications currently
returns restTemplate.exchange(...).getBody() directly which can be null and
cause NPE/IOOBE downstream; update moveModifications (and the other similar
methods that call restTemplate.exchange(...).getBody()) to capture the
ResponseEntity<NetworkModificationsResult>, check responseEntity.getBody() for
null, and if null throw a clear, fast-fail exception (e.g.,
IllegalStateException or a custom exception) with a descriptive message
indicating the remote PUT returned an empty body for the MOVE action and include
context (originGroupUuid, targetGroupUuid); ensure you reference the
restTemplate.exchange call and NetworkModificationsResult so the guard is added
at the client boundary for all analogous methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 996d639b-b58b-4954-a7a4-c2d5a565cee2
📒 Files selected for processing (8)
src/main/java/org/gridsuite/study/server/StudyConstants.javasrc/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/dto/ModificationsToCopyInfos.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/NetworkModificationTest.javasrc/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyControllerRebuildNodeTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/gridsuite/study/server/dto/ModificationsToCopyInfos.java
src/main/java/org/gridsuite/study/server/controller/StudyController.java
Show resolved
Hide resolved
Comment on lines
+2424
to
+2426
| NetworkModificationsResult networkModificationResults = networkModificationService.duplicateModifications(groupUuid, Pair.of(modificationsUuids, modificationApplicationContexts)); | ||
|
|
||
| copyModificationsToExclude(originNodeUuid, targetNodeUuid, modifications, networkModificationResults); | ||
| copyModificationsToExclude(originNodeUuid, targetNodeUuid, modificationsUuids, networkModificationResults); |
There was a problem hiding this comment.
Validate the duplicate response size before copying exclusion tags.
Lines 2426 and 2481-2482 assume the server returns exactly one duplicated UUID per requested source UUID. If one item is missing, this throws; if the contract ever drifts, exclusion tags can be copied onto the wrong duplicate.
Suggested guard
private void copyModificationsToExclude(UUID originNodeUuid,
UUID targetNodeUuid,
List<UUID> modificationsUuids,
NetworkModificationsResult networkModificationResults) {
+ List<UUID> duplicatedModificationUuids = networkModificationResults.modificationUuids();
+ if (duplicatedModificationUuids.size() != modificationsUuids.size()) {
+ throw new IllegalStateException("Duplicate response size mismatch");
+ }
Map<UUID, UUID> mappingModificationsUuids = new HashMap<>();
for (int i = 0; i < modificationsUuids.size(); i++) {
- mappingModificationsUuids.put(modificationsUuids.get(i), networkModificationResults.modificationUuids().get(i));
+ mappingModificationsUuids.put(modificationsUuids.get(i), duplicatedModificationUuids.get(i));
}
rootNetworkNodeInfoService.copyModificationsToExcludeFromTags(originNodeUuid, targetNodeUuid, mappingModificationsUuids);
}Also applies to: 2476-2485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java` around
lines 2424 - 2426, The call to
networkModificationService.duplicateModifications(...) assumes the response has
one duplicated UUID per requested source and then blindly calls
copyModificationsToExclude; update the code around duplicateModifications and
copyModificationsToExclude to first validate that NetworkModificationsResult (or
its duplicates list/map) contains exactly the same number of entries as
modificationsUuids (and that each source UUID maps to a duplicate UUID), and if
the sizes/mapping do not match, fail fast (throw an exception or return an
error) or log and skip to avoid misapplying exclusion tags; apply the same
validation to the other block referenced (around the 2476-2485 area) so you
always map source->duplicate deterministically rather than assuming order.
There was a problem hiding this comment.
Pull request overview
This PR separates composite modification handling from regular network modification operations by introducing a dedicated composite endpoint and simplifying regular copy/move payloads to UUID lists.
Changes:
- Replace
ModificationsToCopyInfospayloads withList<UUID>for regular move/copy/duplicate flows and update impacted services/tests. - Introduce a dedicated
/composite-modificationsendpoint and service path to handle composite-specific insert/split actions. - Remove the
ModificationsToCopyInfosDTO and split action enums intoModificationsActionTypevsCompositeModificationsActionType.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/org/gridsuite/study/server/studycontroller/StudyControllerRebuildNodeTest.java | Updates controller test to pass UUID lists instead of DTOs. |
| src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java | Updates duplication tests to new APIs (duplicateNetworkModifications, duplicateModifications). |
| src/test/java/org/gridsuite/study/server/NetworkModificationTest.java | Adapts move/duplicate tests to UUID list payloads and adds composite insertion test coverage. |
| src/main/java/org/gridsuite/study/server/service/StudyService.java | Refactors duplication flow, adds composite insert/split flow, and centralizes impact notification emission. |
| src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java | Updates REST payload types to UUID lists and adds composite modifications REST call. |
| src/main/java/org/gridsuite/study/server/dto/ModificationsToCopyInfos.java | Removes obsolete DTO. |
| src/main/java/org/gridsuite/study/server/controller/StudyController.java | Updates move/copy endpoint payload type and adds composite-modifications endpoint. |
| src/main/java/org/gridsuite/study/server/StudyConstants.java | Splits action enums for regular vs composite modifications. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/org/gridsuite/study/server/controller/StudyController.java
Show resolved
Hide resolved
src/main/java/org/gridsuite/study/server/controller/StudyController.java
Show resolved
Hide resolved
Meklo
approved these changes
Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Separates composite handling from regular network modification handling